Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First solution #834

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Wojtas1996PL
Copy link

No description provided.

Copy link

@MateuszRostek MateuszRostek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice try! See some required changes and implement them:)

shoppingCartAnna.setUser(anna);
shoppingCartAnna.setTickets(List.of(missionImpossible));

OrderService orderService = (OrderService) injector.getInstance(OrderService.class);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also call getOrdersHistory before completing order.

public interface OrderDao {
Order add(Order order);

Order getByUser(User user);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One user can have multiple Orders.

Suggested change
Order getByUser(User user);
List<Order> getByUser(User user);

Comment on lines 40 to 49
Query<Order> query = session.createQuery("FROM Order o "
+ "LEFT JOIN FETCH o.tickets t "
+ "LEFT JOIN FETCH o.user "
+ "LEFT JOIN FETCH t.movieSession ms "
+ "LEFT JOIN FETCH t.user "
+ "LEFT JOIN FETCH ms.movie "
+ "LEFT JOIN FETCH ms.cinemaHall "
+ "WHERE o.user =:user", Order.class);
query.setParameter("user", user);
return query.uniqueResult();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not create redundant variables, you can set params and get result list directly on the session.createQuery()

return ..........Order.class).setParameter("user", user).getResultList();

Comment on lines 17 to 22
@Inject
private AuthenticationService authenticationService;
@Inject
private OrderDao orderDao;
@Inject
private UserDao userDao;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use these two here:

Suggested change
@Inject
private AuthenticationService authenticationService;
@Inject
private OrderDao orderDao;
@Inject
private UserDao userDao;
@Inject
private OrderDao orderDao;
@Inject
private ShoppingCartService shoppingCartService;

Copy link

@slade13 slade13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

.setParameter("user", user)
.getResultList();
} catch (Exception e) {
throw new DataProcessingException("Can't find a shopping cart by user: " + user, e);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception message is incorrect. It should say 'Can't find orders by user' instead of 'Can't find a shopping cart by user'.

order.setUser(shoppingCart.getUser());
order.setTickets(new ArrayList<>(shoppingCart.getTickets()));
shoppingCartService.clearShoppingCart(shoppingCart);
return orderDao.add(order);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order is not saved in the database, so it can be lost after the method completeOrder is finished. You should add saving the order in the database.

private List<Ticket> tickets;
private LocalDateTime orderDate;
@ManyToOne(fetch = FetchType.LAZY)
private User user;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between Order and User entities is not clearly defined. Consider adding 'cascade' attribute in @manytoone annotation and specify CascadeType as needed.


@Override
public String toString() {
return "Order{"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including associated entities in the toString() method may lead to LazyInitializationException. Consider excluding 'tickets' and 'user' from toString().

@GeneratedValue(strategy = GenerationType.IDENTITY)
private long id;
@OneToMany(fetch = FetchType.LAZY, cascade = CascadeType.PERSIST)
private List<Ticket> tickets;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between Order and Ticket entities is not explicitly mapped. The 'mappedBy' attribute should be used in @onetomany annotation to specify the field that owns the relationship.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants